Normalize if/else blocks - #10270
Conversation
Co-authored-by: Colin Alworth <colin@vertispan.com>
|
As of this comment, samples are at 10807627 from 10821121, 13494 bytes saved, about 0.1% smaller. I'm skeptical about making the new method on JBlock - though it should be used for for/while/do-while blocks as well (though not try blocks). Probably should resolve that one way or the other before merge. The change to printing blocks in JS when an if has an else are more aggressive than they need to be - this need not happen unless the if has a child if, so we could optimize very slightly more here. Options here include keeping nested blocks for ifs manual and introducing a normalization pass rather than a feature of the JsToStringGenerator, or adding a visitor when printing the JS to see if the block is needed. Java 17 change was done to ease test writing, and probably should be done anyway as part of the 2.14 release process. #10240 was also addressed then reverted in this branch, to follow in a later PR. |
| if (jsStatement instanceof JsExprStmt && ((JsExprStmt) jsStatement).getExpression() instanceof JsFunction) { | ||
| JsFunction jsFunction = (JsFunction) ((JsExprStmt) jsStatement).getExpression(); |
There was a problem hiding this comment.
| if (jsStatement instanceof JsExprStmt && ((JsExprStmt) jsStatement).getExpression() instanceof JsFunction) { | |
| JsFunction jsFunction = (JsFunction) ((JsExprStmt) jsStatement).getExpression(); | |
| if (jsStatement instanceof JsExprStmt exprStmt | |
| && exprStmt.getExpression() instanceof JsFunction jsFunction) { |
If we're using Java 17 here, we can simplify the casts a bit.
There was a problem hiding this comment.
Agreed - the change to 17 was mostly for tests, but it could benefit the code in other places too
This reverts commit c70fb47.
niloc132
added
the
ready
| if (body != null) { | ||
| body = visitor.accept(body, true); | ||
| } | ||
| body = JBlock.ensureBlock(getSourceInfo(), visitor.accept(body, false));//TODO no tests fail without this change... |
There was a problem hiding this comment.
Remove TODO after verifying with a test.
| accept(x.getIfExpr()); | ||
| _rparen(); | ||
| JsStatement thenStmt = x.getThenStmt(); | ||
| if (!(thenStmt instanceof JsBlock) && x.getElseStmt() != null) { |
There was a problem hiding this comment.
This should have a comment that we're testing for dangling else and defensively adding {}s to the block.
This is technically not optimal - if there is no possibility of a nested if in the "then" branch, we don't need to do this. The simplest improvement here then would be to allow certain statement types that can never contain an if, like JsEmpty, JsThrow, JsContinue, JsExprStmt, etc, or even some subset of those. As it stands, this can produce a small regression here, such as if(condition){;}else... when the "then" branch is simply a JsEmpty.
There was a problem hiding this comment.
(Second try at this, github ate the first when I dared to unresolve before posting)
I built this out to be more rigorous, we should always optimally omit {}s except when there could be a nested if with no else. Doesn't remove unnecessary blocks from ifs though.
If we end up with more post-optimization normalizations, we might want to group them up outside of the string generator for readability, though this isn't the first we've added here (true/false to !0/!1, etc). Moving these outside of the string generator also would have the disadvantage of not applying the change when printing an AST node to a string for debugging or viewing an AST dump.
See also #10382
3 participants
Fixes a longstanding TODO, and makes it easier for visitors to rewrite contents of blocks in if statements. Originally this change was intended to have no impact on generated JS, but resulted in a dangling-else problem, so changes were required to emit blocks when an if has an else.
Several tests needed to be updated to no longer assume that an unnecessary block was present after parsing.
Fix #10239